Danger - Binomial and Poisson distributions #2385
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Apply Sweep Rules to your PR?
|
| @@ -23,7 +23,7 @@ export const kde = ({ | |||
| let xWidth = kernelWidth ?? nrd0(samples); | |||
| samples = samples.filter((v) => Number.isFinite(v)); // Not sure if this is needed? | |||
| const len = samples.length; | |||
| if (len === 0) return { usedWidth: xWidth, xs: [], ys: [] }; | |||
| if (len === 0 || xWidth === 0) return { usedWidth: xWidth, xs: [], ys: [] }; | |||
There was a problem hiding this comment.
My best guess about this:
xWidthis zero if all samples are identical, but somehow we decided to run kde on them, maybe because there are less than 5 of them (nrd0will return 0 if variance is 0)- so you want to discard them because otherwise
kdedid something weird or failed
If so, I'm not sure if it's the best way to fix the problem (discarding samples seems bad, and this is too entangled with samplesToPointSetDist needs and other quirks). But I also don't have any better suggestions. Maybe this deserves a comment?
| } | ||
|
|
||
| inv(p: number): number { | ||
| throw new Error("Binomial inv not implemented"); |
There was a problem hiding this comment.
I see that these dists and methods really don't get used now; Danger.binomialDist gets immediately transformed to the sampleset, and there's no Sym.binomial yet.
Can you add a comment about that here if you don't want to implement these? For a real symbolic binomial dist, having inv and toPointSetDist is critical, it won't be rendered as a chart otherwise.
Co-authored-by: Vyacheslav Matyukhin <[email protected]>
Co-authored-by: Vyacheslav Matyukhin <[email protected]>
* main: (114 commits) update all remaining deps; remove ops ts fixes update next.js, relay and some other packages ⬆️ Bump nextra-theme-docs from 2.10.0 to 2.13.2 disable update messages for storybook and prisma ⬆️ Bump @storybook/addon-essentials from 7.4.5 to 7.5.2 ⬆️ Bump @codemirror/view from 6.21.2 to 6.21.4 ⬆️ Bump @storybook/addon-actions from 7.4.5 to 7.5.2 ⬆️ Bump tailwindcss from 3.3.3 to 3.3.5 ⬆️ Bump @types/lodash from 4.14.199 to 4.14.200 ⬆️ Bump @storybook/react from 7.4.5 to 7.5.2 ⬆️ Bump nextra from 2.10.0 to 2.13.2 ⬆️ Bump @codemirror/state from 6.2.1 to 6.3.1 ⬆️ Bump @types/webpack-bundle-analyzer from 4.6.1 to 4.6.2 ⬆️ Bump esbuild from 0.18.20 to 0.19.5 ⬆️ Bump @typescript-eslint/eslint-plugin from 6.7.4 to 6.9.1 ⬆️ Bump vite from 4.4.9 to 4.5.0 fix builds - @types/testing-library__jest-dom is not needed anymore concurrency config for ci workflow fix clusterless relative value items autoRun -> autorun ...
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2385 +/- ##
==========================================
- Coverage 71.80% 71.46% -0.34%
==========================================
Files 113 113
Lines 5838 5895 +57
Branches 1174 1180 +6
==========================================
+ Hits 4192 4213 +21
- Misses 1638 1674 +36
Partials 8 8 ☔ View full report in Codecov by Sentry. |
We don't have inv() functions for these. Also, We can't promise to display them correctly - in many cases, KDE will assume that they are partially continuous. It's not clear how to best get around this.
I added these as Danger.binomialDist and Danger.poissonDist. I didn't use
Danger.binomial, because that was already taken.I can add a changeset, but wanted to get a quick take first.
2 Key Questions here: